-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add History Panel #60
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mateusabelli this is such a great and distinctive feature to have ❤️ Thanks for pursuing this PR. I really feel that we could progressively keep working on enhancing this feature.
The delete button does not use the predefined Button component
This is a minor brush we can give at a later time.
After deleting the current image selected, it remains displaying on the screen
I'm thinking something along these lines. We're keeping state for the current image, what if we compare those string on the click handler and if they match, we reset the current image with the placeholder one?
After reaching 10 images on the history panel, it won't start overwriting older ones
Same here, I would abstract the entire logic into a custom hook and just pass the string. The hook internally should determine what to do with the stored strings and the incoming one. If the max number is reached, it should remove the first item and introduce the incoming one.
function addImageToArray(images, newImage) {
// Maybe extract this outside of the function
const MAX_LIMIT = 10;
// Check if the array has reached the maximum limit
if (images.length === MAX_LIMIT) {
// Remove the first image source
images.shift();
}
// Add the new image source to the end of the array
images.push(newImage);
return images;
}
After selecting an image from history, new pastes will be pasted but not displayed automatically
This one is a tricky one. I think the problem might be with the useEffect
. Either try to separate it into two different effects or I would refactor the component to always grab the src from the state. This would also mean that the pasteImage
function should only return the string and then the component should set state and update the ref.
if (imageRef.current){
currentImage.onclick = async () => {
// Prolly we should rename that function because it no longer makes sense to pass the ref
const imageSrc = await pasteImage();
setCurrentImage(imageSrc);
imageRef.current.src = imageSrc;
}
}
Hopefully that makes sense, let me know if you need to talk more about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this PR ready for review? I tested the latest preview built and I still see opportunity for some improvements. Here are a couple of observations:
- Order of items: I would expect that the last item in the panel is the latest image pasted from my clipboard. Currently, the first item of the array is the last image pasted. In my opinion, it is a bit confusing.
- Maxed items doesn't do anything: I would expect that if I reached the maximum number of items in the stored history, the panel would start replacing removing the oldest items to leave space for the recent items to continually keep storing items.
- Pasting an image that is already there: This might be challenging but if I paste an image that has already been pasted, it creates a duplicate in the array. Are the blobs strings different? Not sure but I think this might be a challenge. If the blobs are always unique we might not be able to control this
- Storing: We might have to address this in a separate story but the items aren't kept in local storage. This would be a nice feature.
Let me know what challenges you are facing. Also, if you want me to jump in and help with development, please let me know and push your latest changes to this PR. @mateusabelli
Hello @iamhectorsosa yes its still under development, I'm trying to fix each item in the PR description checkbox, but after some fixes I've found difficulties to extract the shared state to a custom hook and also to refactor the I've left some TODO comments to remind myself of these points, but if you would like to jump in the code I'd be glad to learn how you would approach this. |
@mateusabelli push your latest changes to this PR and I'll give it a shot from my end 🙏🏼 |
Hi @iamhectorsosa thank you, those are my latest stable changes. I left comments around the code but if you find anything confusing please let me know. If you would like, I'm available to continue the PR after we overcome those challenges. Thanks again! |
✨ Description
This PR will add a history panel to display the previous pasted images from the clipboard. It still at an early stage and it doesn't retrieve the editing state from each image yet. Other than that, everything is quite functional.
To achieve this feature I created a shared state between the component responsible for displaying the image and my new component responsible for displaying the history panel. This implementation might not be according to the project specs, so I'm open for change reviews.
I added comments to help understand my thought process while developing this feature. I am a bit rusty with React, so any feedback is appreciated.
Fixes #13
🖼 Demonstration
🧪 To be fixed
Button
component📋 Notes
After working with the code for some hours, I learned more about how it worked, how it interacts with the clipboard and how the browser gives a nice and clean URL of the blob.
With this information I don't feel like my comment on the issue #13 would be provide the most elegant solution at the moment.
What I said there could work but so far I'm seeing this simpler solution beating my complex thought back there. I feel like that should be left to be built upon something like this, so there would be much less friction during implementation.